[manager] add cache location lookup metrics - #308
Conversation
|
|
![qoderai[bot]](https://avatars.githubusercontent.com/in/1396158?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR adds clear, instance-scoped observability for cache-location lookups and wires the new counters through the KMonitor reporter. The test coverage for the prefix-match error classification, invalid-instance cardinality guard, instance lifecycle, and backend filtering paths is good.
Findings
- Invalid requests may be counted.
LocationLookupMetricsGuardis constructed before thequery_typeand backend-selector validation checks in bothGetCacheLocationandGetCacheLocationsByBackend. Early returns from those checks will still emitmanager.location_lookup.requests_totalandmanager.location_lookup.keys_total{error}, which conflicts with the stated goal of counting valid location lookups. - Guard default key count can misclassify errors. The guard is initialized with
keys.size()and only updated to the real lookup count after validation. If a query fails before that update (e.g., prefix match with a mask or token-generated keys), the destructor records too manyerrorkeys. - Lifecycle lock scope is wide. The
shared_lockonmetrics_lifecycle_->mut_is held for the entire request, including the metadata read and event publish. That guarantees the instance cannot be removed while metrics are written, but it also blocksRemoveInstancebehind active backend reads. Consider scoping the lock to the metric guard lifetime.
Suggestions
- Move
LocationLookupMetricsGuardconstruction (and the lifecycle lock) to after all request validation, and initialize it with the actuallookup_key_count. - If the wide lock scope is intentional, add a brief comment explaining why it must cover the whole request rather than just the metric write.
No blocking issues—happy to re-review after the guard placement is tightened.
🤖 Generated by Qoder
| RETURN_IF_EC_NOT_OK_WITH_TYPE_LOG( | ||
| WARN, EC_INSTANCE_NOT_EXIST, CacheLocationViewVecWrapper, "instance not found"); | ||
| } | ||
| LocationLookupMetricsGuard location_metrics( |
There was a problem hiding this comment.
LocationLookupMetricsGuard is created before the query_type == QT_UNSPECIFIED check. If that check fails, the guard destructor still emits manager.location_lookup.requests_total and classifies all keys.size() keys as result=error. Since the PR description says requests_total counts valid lookups, consider moving the guard (and the lifecycle lock) after all request validation.
🤖 Generated by Qoder
| if (instance_info == nullptr) { | ||
| RETURN_IF_EC_NOT_OK_WITH_TYPE_LOG(WARN, EC_INSTANCE_NOT_EXIST, BatchLocationsView, "instance not found"); | ||
| } | ||
| LocationLookupMetricsGuard location_metrics( |
There was a problem hiding this comment.
Same guard-placement issue here: the guard is constructed before validating query_type, block mask, location_spec_names, and backend_selectors. Any of those failures will emit requests_total and keys_total{error} for a request that never reached the lookup path. Constructing the guard after validation and initializing it with the real lookup_key_count would avoid counting invalid requests.
🤖 Generated by Qoder
| // Keep a valid instance alive until the dynamic metric write completes. | ||
| // RemoveInstance holds the same lifecycle fence exclusively before it | ||
| // purges instance-tagged series. | ||
| std::shared_lock<std::shared_mutex> metrics_lifecycle_guard(metrics_lifecycle_->mut_); |
There was a problem hiding this comment.
The shared_lock is held for the whole request, including the metadata query, filtering, and event publish. This correctly prevents RemoveInstance from purging instance-tagged series while metrics are being written, but it also serializes instance removal behind active metadata reads. Consider scoping the lock to just the metric guard lifetime (validation → Complete) so removals are not blocked by backend latency. If the wider scope is intentional, a short comment would help future readers.
🤖 Generated by Qoder
| const std::size_t lookup_key_count = query_type == QueryType::QT_PREFIX_MATCH | ||
| ? CountUnmaskedKeys(query_keys, block_mask) | ||
| : query_keys.size(); | ||
| location_metrics.SetRequestKeyCount(lookup_key_count); |
There was a problem hiding this comment.
If PerformCacheLocationQuery returns an error and the function returns before this line, the guard destructor records error keys using the initial keys.size() rather than the masked/generated lookup count. For prefix match with a mask or token-generated keys this over-counts error keys. Moving guard construction after validation and initializing it with the actual lookup_key_count would keep the error count consistent.
🤖 Generated by Qoder
2 participants
Summary
manager.location_lookup.requests_totalto count valid location lookup requestsmanager.location_lookup.request_keys_totalto count queried cache block keysmanager.location_lookup.keys_total, split byresult=hit|miss|filtered|error, to expose lookup outcomesThese counters provide the inputs needed to derive the cache-location hit rate in Grafana. The derived hit rate represents metadata location hits and does not imply that the cache payload was read successfully.
Validation